Sktime and Pyspi Integration#82
Sktime and Pyspi Integration#82Spinachboul wants to merge 7 commits intoDynamicsAndNeuralSystems:mainfrom
Conversation
| def _spi(self, data: Data, i: int = 0) -> float: | ||
| raise NotImplementedError("Subclass must implement _spi.") | ||
|
|
||
| @parse_univariate |
There was a problem hiding this comment.
I would not use these as decorators at all! But move the logic as a call into spi.
|
@joshuabmoore Does the current API design for the class align well. |
|
@fkiraly |
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
I would recommend you leave the current base classes untouched to ensure downwards compatibility. Add the new base class and try to write three substantially different spi as a descendant, include example usage in the concrete class docstring.
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
- the original base class is still changed - can you revert that for now?
- the new base class looks mostly reasonable - I would suggest to move ahead and try implement two concrete SPI (perhaps slightly different ones).
- also, try to add tests for the SPI. For now, it is fine to loop over the concrete classes (once you have them)
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Great! This fixes the point with reverting changes to the old base class.
Not addressed yet:
- try implement two concrete SPI (perhaps slightly different ones).
- try to add tests for the SPI. For now, it is fine to loop over the concrete classes (once you have them)
|
@fkiraly @benfulcher @joshuabmoore Thanks!! |
1 similar comment
|
@fkiraly @benfulcher @joshuabmoore Thanks!! |
|
@fkiraly @joshuabmoore @benfulcher Thanks!! |
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Looks nice!
I would recommend next:
- add tests
- add at least two SPI which are already in the code base
2 participants
@benfulcher @joshuabmoore @fkiraly
Solving issue #72
This initial implementation will be the starter modified base class of pyspi, and an attempt to make a unified public API. Keeping it draft until I finalize commits
These will be the scope of the upcoming commits:
BaseSPI,DirectedandUndirected.spiandspi_matfunctions.Your reviews and guidance would be highly appreciated in this PR Thanks a lot!!